Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable file-backed caching on webGL platform #37

Merged
merged 11 commits into from
Aug 4, 2023
Merged

Disable file-backed caching on webGL platform #37

merged 11 commits into from
Aug 4, 2023

Conversation

aherbig
Copy link
Contributor

@aherbig aherbig commented Aug 3, 2023

Hey @StephenHodgson,

as mentioned on discord the current implementation of the DownloadCache was running into trouble on the webGL platform.

With the changes I implemented here I tried to separate the caching so that the DownloadCache can be replaced depending on the platforms needs.

I originally wanted the DownloadCache to already return the assets instead of just returning the URLs to load, so that on webGL there could be a runtime-cache for the textures and audioClips. I decided against that for now, because I would have needed to change more of your existing code which I don't wanna do without your consent.

Would be glad if these changes will go live promptly.

@aherbig aherbig requested a review from StephenHodgson as a code owner August 3, 2023 10:00
Copy link
Member

@StephenHodgson StephenHodgson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do an in-depth review yet, but I want to say that I like the abstraction you added for cache 💜

@StephenHodgson StephenHodgson changed the base branch from main to development August 4, 2023 23:09
@StephenHodgson StephenHodgson merged commit 8e0da2f into RageAgainstThePixel:development Aug 4, 2023
@aherbig
Copy link
Contributor Author

aherbig commented Aug 7, 2023

I commented on one of your commits but it seems the messages has disappeared.

So I will write it again.

I wanted to know why you want to keep the code from the "revert some cache checks" commit (5fa6946).

In concrete implementation of the DownloadCache does already check if an item is cached or not, and is doing the exact same check with an early return:

// From DiskDownloadCache.cs:

        public bool TryGetDownloadCacheItem(string uri, out string filePath)
        {
            ValidateCacheDirectory();
            bool exists;

            if (uri.Contains(Rest.FileUriPrefix))
            {
                filePath = uri;
                return File.Exists(uri.Replace(Rest.FileUriPrefix, string.Empty));
            }
       }

I would argue that it should be up to the DownloadCache to decide if an item is cached or not.

@aherbig aherbig deleted the feature/webgl-cache branch August 7, 2023 10:30
@StephenHodgson
Copy link
Member

In concrete implementation of the DownloadCache does already check if an item is cached or not, and is doing the exact same check with an early return

It was a breaking change and the early check actually was needed to prevent unintended behavior changes to existing packages.

@aherbig
Copy link
Contributor Author

aherbig commented Aug 8, 2023

Oh, got you. Good that you spotted that then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants